PT-4214 Stage U: backend-authoritative per-project S/R block signal (E2E defects 2+3) - #2574
Conversation
… oldest-first pairing Review findings 1-3 on #2571: pairing anonymous clears with the oldest outstanding leash both releases a live blocker (a late clear after a leash fired cancels a newer switch's leash) and starves an abandoned blocker's own leash (every clear cancels the leash nearest to firing, so blocking persists indefinitely under ongoing traffic). Both stores now hand out one release token per block/switch. A token is released exactly once - by its own clear or its own leash - and a late clear after the leash fired is a no-op by identity. The dual counter and the timer-array scan dissolve (findings 14/15): the live-token set is the single source of truth. The project-switch events now carry a switchId (declared in NetworkEvents, emitted by platform-scripture-editor), so the workspace-updating service pairs every finish with exactly the switch that started it - both review traces are fixed end-to-end and pinned by integration tests that fail on the old code. The auto-sync blocking event remains an anonymous boolean, so that service keeps oldest-first pairing with tombstones for leash-released raises (fixes the late-clear trace); the abandoned-raise trace is unfixable on an anonymous wire and is resolved by the backend-authoritative snapshot model in #2574, as documented in the service. Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the command is registered nowhere at this tip; the doomed consult cost ~9 s of main-process retry churn per renderer launch because requestNoRetry's flag does not cross the wire - now documented on requestNoRetry itself). The "Known limitation" reload note is restored, pointing at PT-4214, whose C# SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting the seed also makes initAutoSyncBlockingService synchronous again, so the index.tsx startup comment is accurate at this tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
756d33e to
08d0d3d
Compare
08d0d3d to
22081b9
Compare
37d955d to
b99d516
Compare
… oldest-first pairing Review findings 1-3 on #2571: pairing anonymous clears with the oldest outstanding leash both releases a live blocker (a late clear after a leash fired cancels a newer switch's leash) and starves an abandoned blocker's own leash (every clear cancels the leash nearest to firing, so blocking persists indefinitely under ongoing traffic). Both stores now hand out one release token per block/switch. A token is released exactly once - by its own clear or its own leash - and a late clear after the leash fired is a no-op by identity. The dual counter and the timer-array scan dissolve (findings 14/15): the live-token set is the single source of truth. The project-switch events now carry a switchId (declared in NetworkEvents, emitted by platform-scripture-editor), so the workspace-updating service pairs every finish with exactly the switch that started it - both review traces are fixed end-to-end and pinned by integration tests that fail on the old code. The auto-sync blocking event remains an anonymous boolean, so that service keeps oldest-first pairing with tombstones for leash-released raises (fixes the late-clear trace); the abandoned-raise trace is unfixable on an anonymous wire and is resolved by the backend-authoritative snapshot model in #2574, as documented in the service. Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the command is registered nowhere at this tip; the doomed consult cost ~9 s of main-process retry churn per renderer launch because requestNoRetry's flag does not cross the wire - now documented on requestNoRetry itself). The "Known limitation" reload note is restored, pointing at PT-4214, whose C# SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting the seed also makes initAutoSyncBlockingService synchronous again, so the index.tsx startup comment is accurate at this tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
234e46a to
0bdfd05
Compare
lyonsil
left a comment
There was a problem hiding this comment.
A few review comments below, from an AI-assisted review pass with my guidance. These are a curated subset of findings, mostly cleanups plus a couple of correctness notes (the lost-disarm recovery gap and the renderer case-sensitivity). Happy to discuss any of them.
(AI-assisted, with my guidance)
|
Re-review note on a cleanup that can't be anchored inline (its file isn't in this PR's diff):
(AI-assisted, with my guidance) |
… oldest-first pairing Review findings 1-3 on #2571: pairing anonymous clears with the oldest outstanding leash both releases a live blocker (a late clear after a leash fired cancels a newer switch's leash) and starves an abandoned blocker's own leash (every clear cancels the leash nearest to firing, so blocking persists indefinitely under ongoing traffic). Both stores now hand out one release token per block/switch. A token is released exactly once - by its own clear or its own leash - and a late clear after the leash fired is a no-op by identity. The dual counter and the timer-array scan dissolve (findings 14/15): the live-token set is the single source of truth. The project-switch events now carry a switchId (declared in NetworkEvents, emitted by platform-scripture-editor), so the workspace-updating service pairs every finish with exactly the switch that started it - both review traces are fixed end-to-end and pinned by integration tests that fail on the old code. The auto-sync blocking event remains an anonymous boolean, so that service keeps oldest-first pairing with tombstones for leash-released raises (fixes the late-clear trace); the abandoned-raise trace is unfixable on an anonymous wire and is resolved by the backend-authoritative snapshot model in #2574, as documented in the service. Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the command is registered nowhere at this tip; the doomed consult cost ~9 s of main-process retry churn per renderer launch because requestNoRetry's flag does not cross the wire - now documented on requestNoRetry itself). The "Known limitation" reload note is restored, pointing at PT-4214, whose C# SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting the seed also makes initAutoSyncBlockingService synchronous again, so the index.tsx startup comment is accurate at this tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b99d516 to
e0f1b19
Compare
|
Follow-up (non-blocking): mark the new #2571 adds two new PAPI network events that carry through this PR unmarked (
Per
Not blocking #2571 — it's otherwise ready to merge and I'd rather not add churn there. But this shouldn't reach a released build unmarked, so let's fold it in before the stack is fully merged; this PR already touches these files, so it's a natural home. One honest caveat: the sibling events ( (AI-assisted, with my guidance) |
|
Follow-up (non-blocking): drop backward-facing "review finding" references in the workspace-updating tests before the stack merges Several of the new regression tests tag their names/docs with the review that motivated them ("review finding N", "the #2571 review"). Those are backward-facing: the scenario each test describes is already self-contained, so the review reference adds nothing for a future reader and only goes stale once this work merges. On this branch they survive in the workspace-updating tests:
Each reads cleanly with the trailing tag simply removed — the behavior is fully spelled out in the name/body already, e.g. the integration TSDoc becomes "...pin the identity pairing between a switch's leash and its own finish." The auto-sync-store equivalents that carried the same tags on #2571 self-resolve here: this PR deletes For contrast, the Not blocking — same spirit as the experimental-events note above: cheap to fold in here (these files are already in this PR) so the work doesn't reach a merged state carrying review-process history. (AI-assisted, with my guidance) |
0bdfd05 to
2b4e6c2
Compare
- Canonicalize project ids to upper at ingestion + in both readers (isEditorBlocked, isProjectBlocked) so a casing skew can't leave a project silently unblocked - applyBlockedSetToAllEditors reports success; syncState only advances the applied snapshot on a successful apply (open-but-unenumerable editors no longer recorded as flagged) - Subscribe to the gate before the event-registration round-trip so no transition is dropped - Move SendReceiveBlockState to its own file (PNX004); add a shared static NotBlocking snapshot; drop the redundant ToArray copy in GetBlockState - Reuse deepEqual/isString from platform-bible-utils; scope the dead rawBlockedProjectIds writes to the arming branch - Add a pointer comment noting the init consult is the only backend re-seed (lost-disarm recovery tracked on PT-4214) - Fix the now-stale workspace-updating-store cross-reference comment Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Claude: Thanks Matt — went through all of them, addressed in 65247b7. Correctness notes taken:
Cleanups applied: One deliberate keep: I kept Deferred with tracking (both on PT-4214): the Also fixed the now-stale |
…Project @experimental Per lyonsil's #2574 follow-up: the two new switch-pairing network events (added in #2571) are recently-added PAPI surface whose contract isn't settled. Add per-member @experimental TSDoc (extension .d.ts isn't run through TypeDoc, so each member needs its own tag). Matches the posture of AUTO_SYNC_MAX_DURATION_MS, which #2571 marked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ia async emitter with x-experimental Part 2 of lyonsil's #2574 @experimental follow-up: switch the onWillSwitchProject / onDidSwitchProject emitters from the deprecated sync createNetworkEventEmitter to createNetworkEventEmitterAsync, passing notification docs with 'x-experimental': true so the events are marked experimental in the generated OpenRPC document (not just the .d.ts TSDoc). The enclosing open() handler is already async, so awaiting emitter creation only adds first-use latency and the will-start event still fires before the switch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lyonsil
left a comment
There was a problem hiding this comment.
The fixes for the previously reported issues look good. However, I see many comments referencing "PT-4214 Stage U" that are backward-facing explanations justifying a change, but they have little to no context for future maintainers. We should only leave comments that are forward-facing, helpful documentation and references for future maintainers. I flagged one example as I'm sure Claude will do a better job of reading all the comments in the PR and reformulating them to be forward-facing only.
@lyonsil reviewed 20 files and all commit messages, made 2 comments, and resolved 14 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on rolfheij-sil).
src/shared/data/platform.data.ts line 29 at r2 (raw file):
* deliberately long. * * As of PT-4214 Stage U the renderer auto-sync-blocking store no longer consumes this: the backend
Example of "Stage U" backward-facing comment
…(lyonsil #2574 review) Sweep the PR's comments per lyonsil's change request: drop change-history / 'PT-4214 Stage U' / 'review finding' / E2E-date justifications and restate each as present-tense documentation of what the code does. Keep genuine forward pointers to open follow-ups (editor-mount re-query, gate arming). papi.d.ts regenerated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Claude: Both done — comment sweep + the earlier Comment sweep (this change request), in
|
lyonsil
left a comment
There was a problem hiding this comment.
@lyonsil reviewed 10 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
…lock-state change event (PT-4214 Stage U)
Reverse SendReceiveWriteLock's rejection from a process-wide global gate to a
per-project one: while a set of projects is syncing, EnterWrite now rejects only
writes to THOSE projects — a write to any other project proceeds. This is a
deliberate product requirement (PT9 parity: a sync locks only the project it is
syncing, so a user can keep editing project B while project A syncs). Both
EnterWrite and IsBlocked now consult _blockedProjectIds; the core mutual-exclusion
invariant is unaffected (narrowing rejection can only allow more writes, and the
count++/arm ordering still bars any write to a synced project from racing that
project's file replacement). The read of the blocked set is safe against tearing
because SetSyncing publishes it data-then-flag, so a reader that observes the armed
bit always observes the matching set.
Add a backend-authoritative change-notification surface on the gate:
- public event Action<SendReceiveBlockState>? BlockStateChanged, raised after a
successful arm (with the armed ids) and after any real disarm (empty); no-op
Clear()/stale-token Clear(long) do not raise. Subscriber faults are swallowed
(this pure class has no logger; the notifier owns real error handling).
- public GetBlockState() returning a best-effort snapshot from the armed flag + set.
- readonly record struct SendReceiveBlockState(IsBlocking, ProjectIds) — serializes
to the exact { isBlocking, projectIds } wire shape via the shared camelCase PAPI
JSON options.
SetSyncing/Clear/Clear(long)/EnterWrite signatures are unchanged (the in-flight
studio patch PR #164 calls them). Drain semantics stay GLOBAL. The class doc is
rewritten to describe the per-project reversal and the two benign flag/set-skew
windows, both of which resolve to "allow". ResetForTests now also clears
BlockStateChanged subscribers so tests can't leak subscriptions.
Tests: replace the global-gate test with per-project tests (unsynced project
allowed with a working scope that participates in a later drain; synced projects
rejected); fix the nesting-hazard test to use the same project (per-project filter
no longer rejects a different one); add event tests (arm/re-arm/disarm raise, stale
and no-op Clear do not, throwing subscriber can't break arm/clear) and GetBlockState
snapshot tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…write gate to the PAPI (PT-4214 Stage U)
Introduce a startup service that forwards SendReceiveWriteLock's block-state
transitions to the renderer so the UI has a backend-authoritative view of whether
an automatic Send/Receive is blocking edits, and for which projects:
- subscribes to SendReceiveWriteLock.BlockStateChanged and fire-and-forgets a
paratextBibleSendReceive.onSyncWriteLockChanged PAPI event (try/catch + log,
mirroring SharedStore.Set's event-send handling).
- registers command:paratextBibleSendReceive.getAutoSyncBlocking returning the
current GetBlockState() snapshot so a renderer can seed on demand.
Both carry the same { isBlocking, projectIds } wire shape.
Wire the service into Program.cs's startup Task.WhenAll alongside the other PAPI
services. Inert in open-source Platform.Bible: nothing arms the gate there, so the
event never fires and the command always returns not-blocking. Because PapiClient
has no network:registerEvent counterpart, the event is an unregistered announcement
following the existing SharedStore STORE_CHANGE_EVENT precedent (TODO PT-4214
follow-up when C# gains registerEvent).
Tests: SendReceiveBlockNotifierServiceTests verifies the command is registered, a
gate arm/clear pushes the event with the right name + snapshot payload, the command
returns the current snapshot, and SendReceiveBlockState serializes to the camelCase
wire shape. Add a DummyPapiClient.InvokeRequestHandler test helper to invoke a
locally-registered handler without a live PAPI connection.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…hot (PT-4214 Stage U) Replace the ref-counted boolean model with a backend-authoritative SNAPSHOT model: the store now holds the set of project ids an automatic Send/Receive is blocking edits on, replaced wholesale by the producer via the new setBlockedProjects(). Empty set = not blocking. Add per-project accessors getBlockedProjectIds() and isProjectBlocked() (undefined project id -> false) alongside the preserved getAutoSyncBlocking() (any project blocked) and subscribeToAutoSyncBlocking(). The 200 ms show-grace debounce for the derived visible flag is kept verbatim (PT9-parity UX), and listeners now notify on any visible-set content change (so a project joining or leaving an in-flight batch is observable), not just a boolean flip. Delete the per-blocker safety-leash timer array and all AUTO_SYNC_MAX_DURATION_MS usage in the store. This is the ratified PT-4214 5.2 decision: the renderer's SAFETY_TIMEOUT_MS timer is deleted, not retained -- a second, timer-driven opinion about blocking is precisely the drift findings 7/8/16 indict. Renderer resilience against a lost event is now re-query of the backend authority (the service's init consult), not a local timer. These leashes were added by Stage T on this very base branch (7a103c7); superseding them here is deliberate. AUTO_SYNC_MAX_DURATION_MS is kept in platform.data.ts (shutdown-tasks.ts still uses it); its doc comment is updated to record that the renderer-store consumer is gone per Stage U and the constant's remaining rightful homes are the shutdown-sync bound and, conceptually, the C# stall watchdog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…event (PT-4214 Stage U)
Subscribe the auto-sync-blocking service to the C# backend's
paratextBibleSendReceive.onSyncWriteLockChanged event -- a full { isBlocking,
projectIds } snapshot emitted on every gate arm/disarm for ALL sync types
(manual + scheduled + session) -- and forward it to the store's new
setBlockedProjects(isBlocking ? projectIds : []).
Remove the subscription to paratextBibleSendReceive.onAutoSyncBlockingChanged
entirely: the backend gate is now the single signal source (PT-4214 finding 16);
a second renderer-side signal is exactly the drift that finding indicts. A short
comment names the replaced event and why.
Parse the snapshot defensively: a malformed or missing-field payload is treated
as block-none and warns once per service lifetime (fail-safe assume-unblocked,
consistent with the existing init-seeding philosophy -- a broken signal must
never leave editors stuck read-only).
Keep the init consult of paratextBibleSendReceive.getAutoSyncBlocking
(requestNoRetry; it is C#-served now) but parse the NEW snapshot shape, keep the
hasReceivedEvent live-event-wins race guard, and keep the fail-safe
assume-unblocked on rejection (plain Platform.Bible serves the command but
returns not-blocking; older cores lack it and the request rejects).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…4 Stage U)
Make the auto-sync edit-block driver per-project. A Scripture editor web view is
flagged isSyncBlocked iff its definition.projectId is in the store's blocked set;
editors with no projectId, or whose project is not syncing, are never flagged.
This is the fix for E2E defect 3: syncing project A must not block edits in an
open editor of project B.
The driver now reacts to SET CHANGES (backend snapshots) rather than a boolean
raise/clear. On each store notification it diffs the new blocked set against the
last-applied one (no-op when unchanged) and re-applies desired state to every
open editor; the per-editor write-equality guard turns the re-apply into a
minimal diff (flag newly-blocked projects, unflag no-longer-blocked ones).
Preserve the two hard-won behaviors, generalized to per-project:
- mid-block subscriptions: onDidOpenWebView flags a newly-opened editor whose
project is blocked; onDidUpdateWebView re-flags a rebuilt editor of a blocked
project (the editor factory forces isSyncBlocked:false on rebuild).
- THE ORDERING FIX: on any transition, tear the re-flag/open handlers down
BEFORE applying the diff, because updateWebViewDefinitionSync fires
onDidUpdateWebView synchronously and a live re-flag handler would observe its
own unflag write and re-flag permanently. Chosen shape: unsubscribe handlers,
apply the full diff, then resubscribe over the new set if it is non-empty.
This is correct for PARTIAL transitions too ({A,B} -> {A}): B's editors unflag
with no live handler to bounce them, A's editors keep their flag, and the
rebuilt {A} handlers still protect A on a later rebuild.
Adapt the regression tests to per-project semantics (projectId-carrying editors,
getBlockedProjectIds set mock): defect-3 two-project isolation, partial shrink
{A,B}->{A}, own-unflag-must-not-re-flag incl. partial transitions, rebuilt
re-flag only for the blocked project, no-projectId editor never flagged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…er-project docs, observed notifier faults (PT-4214)
Adversarial review of the per-project write-gate found 5 low-severity
doc/behavior-description issues:
- SetSyncing's all-invalid-batch comment/warning claimed an armed empty
set "rejects writes exactly like any other arm". Under the per-project
EnterWrite check (armed && set.Contains(id)) that's false: an empty
set matches nothing, so such an arm blocks NOTHING. Rewrote the
comment/warning to say so, and pinned it with a new NUnit test
(SetSyncing_AllInvalidBatch_ArmsButBlocksNoProject).
- SetSyncing's XML doc still described the old global-gate behavior
("EnterWrite calls for ANY project fail fast", "arms the global
gate"); reworded to per-project language consistent with the class
remarks/EnterWrite/IsBlocked docs.
- Added an honest paragraph to the BlockStateChanged doc: raises happen
outside the atomic transition and carry no sequence stamp, so the two
documented off-contract races (any-thread crash-recovery Clear(), or
a takeover racing a stale Clear(token)) can deliver events out of
order; self-corrects at the next transition. No sequence stamp added
(YAGNI under the serialized-scheduler contract).
- SendReceiveBlockNotifierService's try/catch around the discarded
`_ = PapiClient.SendEventAsync(...)` task was dead: SendEventAsync is
async, so no exception from it can ever throw synchronously, only
land unobserved on the discarded Task. Switched to
ThreadingUtils.RunTask (the existing repo pattern for observing a
fire-and-forget task's fault) so a failed send is actually logged,
and documented the deliberate divergence from SharedStore.Set's
same-shaped (and equally dead) precedent.
- Repaired a garbled sentence in the class doc's "one atomic word"
paragraph.
Also checked the getNetworkEvent<T> deprecation flagged in
auto-sync-blocking-service.ts:80 — the base branch (7a103c7)
already used the same deprecated explicit-type-parameter overload for
the same reason (the event isn't declared in the public NetworkEvents
map), so left it unchanged to match surrounding code.
dotnet test c-sharp-tests --filter FullyQualifiedName~SendReceive:
66 passed, 0 failed. csharpier --check clean on touched files.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…ition driver test (PT-4214)
getAutoSyncBlocking() had no consumers outside its own test (the papi command
of the same name is served independently by the C# backend); isProjectBlocked
is what the store's real consumers use, plus the upcoming project-settings UI.
Also pins the {A}->{B} pure-swap transition with a driver test, confirming a
stale onDidUpdateWebView from the swapped-out project's editor cannot bounce
it back to blocked.
… registry (PT-4214) The block-state event was announced unregistered, drawing main's boot-time deprecation warning like every C#-origin event. No new API is needed, though: network:registerEvent is an ordinary main-process JSON-RPC method served to every websocket client, and PapiClient's generic SendRequestAsync can call it generically — the exact way RegisterRequestHandlerAsync already calls network:registerMethod. InitializeAsync now registers the event before subscribing to the gate, making the C# connection the event's single registered source, so announcements pass the registry check cleanly. Best-effort: rejection or failure is logged and emitting continues, because announcing unregistered still works (main warns once) and backend startup must never break over a registry hiccup. DummyPapiClient now captures outgoing wire requests (and accepts network:registerEvent, mirroring ConnectAsync's "pretend we succeeded") so a test pins the registration. Other C#-origin events (-pdp-data:onDidUpdate, shared-store:change) stay on the legacy announce path pending the platform-wide migration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebase reconciliation on top of #2571's identity-pairing fix round: drop the anonymous-boolean integration test whose event seam (onAutoSyncBlockingChanged + raise/clear pairing) this branch deletes - the snapshot store makes the pairing problem it pinned unrepresentable - and update the index.tsx startup comment, which #2571 made accurate again by cutting the inert seeding: this branch reintroduces the (now C#-served) init consult, so the services are no longer both synchronous (review finding 13 on #2571). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SendReceiveBlockNotifierService's InitializeAsync registers the onSyncWriteLockChanged event with main's central registry best-effort: on rejection (accepted == false) it warns and continues, and on a thrown registration it catches, logs, and continues — in both cases still registering the getAutoSyncBlocking command and still emitting gate transitions. DummyPapiClient hard-coded network:registerEvent -> true, so neither branch was exercised; the contract was asserted only by inspection. Make the dummy's register response configurable via a RegisterEventResponse Func<bool> (default unchanged: returns true), and add two tests pinning the best-effort contract: (a) registry rejects (false) -> warning logged, command still registered, gate transition still emitted; (b) registration throws -> caught and logged (with the exception detail), command still registered, transition still emitted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe
- Canonicalize project ids to upper at ingestion + in both readers (isEditorBlocked, isProjectBlocked) so a casing skew can't leave a project silently unblocked - applyBlockedSetToAllEditors reports success; syncState only advances the applied snapshot on a successful apply (open-but-unenumerable editors no longer recorded as flagged) - Subscribe to the gate before the event-registration round-trip so no transition is dropped - Move SendReceiveBlockState to its own file (PNX004); add a shared static NotBlocking snapshot; drop the redundant ToArray copy in GetBlockState - Reuse deepEqual/isString from platform-bible-utils; scope the dead rawBlockedProjectIds writes to the arming branch - Add a pointer comment noting the init consult is the only backend re-seed (lost-disarm recovery tracked on PT-4214) - Fix the now-stale workspace-updating-store cross-reference comment Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Project @experimental Per lyonsil's #2574 follow-up: the two new switch-pairing network events (added in #2571) are recently-added PAPI surface whose contract isn't settled. Add per-member @experimental TSDoc (extension .d.ts isn't run through TypeDoc, so each member needs its own tag). Matches the posture of AUTO_SYNC_MAX_DURATION_MS, which #2571 marked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ia async emitter with x-experimental Part 2 of lyonsil's #2574 @experimental follow-up: switch the onWillSwitchProject / onDidSwitchProject emitters from the deprecated sync createNetworkEventEmitter to createNetworkEventEmitterAsync, passing notification docs with 'x-experimental': true so the events are marked experimental in the generated OpenRPC document (not just the .d.ts TSDoc). The enclosing open() handler is already async, so awaiting emitter creation only adds first-use latency and the will-start event still fires before the switch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(lyonsil #2574 review) Sweep the PR's comments per lyonsil's change request: drop change-history / 'PT-4214 Stage U' / 'review finding' / E2E-date justifications and restate each as present-tense documentation of what the code does. Keep genuine forward pointers to open follow-ups (editor-mount re-query, gate arming). papi.d.ts regenerated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c6a3382 to
cfa3390
Compare
lyonsil
left a comment
There was a problem hiding this comment.
@lyonsil reviewed 5 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
…king, onSyncWriteLockChanged) + public breakSyncLock stub (#2612) * PT-4214: declare the S/R write-gate seam API held out of #2574 Declares in the paratext-bible-send-receive type seam what core PR #2574 shipped undeclared (held back to avoid colliding with #2570's seam re-home, which has now merged): - SyncWriteLockSnapshot payload type (the C# SendReceiveBlockState wire shape) - paratextBibleSendReceive.getAutoSyncBlocking command - paratextBibleSendReceive.onSyncWriteLockChanged network event All three are tagged @experimental. The renderer auto-sync-blocking service now consumes the seam instead of its interim local types: imports nothing locally-duplicated, subscribes through the typed getNetworkEvent overload, and sends the init consult through the typed sendCommand instead of an untyped requestNoRetry (identical wire behavior from the renderer, where the no-retry flag never crossed the wire anyway). The fail-safe payload validation stays, since older or off-contract cores remain possible. The legacy onAutoSyncBlockingChanged event was never declared in core, so there is nothing to remove; the test asserting non-subscription is kept. Regenerating papi.d.ts produces no changes - the src/@types seam is not part of papi.d.ts generation. Claude-Session: https://claude.ai/code/session_019MhYcp1XVTQ7JHiA4n2qaV Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * PT-4210 Task 3: public breakSyncLock stub + seam declaration paratextBibleSendReceive.breakSyncLock so far existed only in the private Paratext 10 Studio patch, deviating from the public/private seam convention where core ships the public scaffolding (stub + declared contract) and the patch only fills in the implementation. - ParatextProjectSendReceiveService: register the command and add a BreakSyncLock stub throwing PlatformUnimplementedException, following the cancelSync/syncProjects pattern and phrasing. The signature matches the patch's implementation (List<string> projectIds -> Task<Dictionary<string, bool>>) so the patch converts to a body fill; the stub is non-async to avoid CS1998, so the patch also adds the async keyword. The patch-only s_sendReceiveTimeout registration argument stays in the patch, like syncProjects. - Seam d.ts: declare the command in CommandHandlers, copied from the Send/Receive extension's own declaration, plus the seam's standard PlatformUnimplementedException @throws note. Tagged @experimental. No core registration-list test or doc enumerates these commands, so nothing else to extend. Claude-Session: https://claude.ai/code/session_019MhYcp1XVTQ7JHiA4n2qaV Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * PT-4210: address review round — wire experimental docs, PT-4265 pointer, breakSyncLock doc clarifications - breakSyncLock and getAutoSyncBlocking registrations now pass ExperimentalMethodDocumentation so the x-experimental marker reaches the wire (OpenRPC rpc.discover), per the Experimental APIs standard. - Update the editor-mount re-query tracking pointer from the closed PT-4214 to its surviving ticket PT-4265. - breakSyncLock docs (TS + C# XML) now state it breaks the SERVER-side repository lock and is unrelated to the local in-process write gate reported by onSyncWriteLockChanged / getAutoSyncBlocking. - Document breakSyncLock's empty-array behavior (verified against the real implementation): empty input is a no-op — empty result map, server never contacted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MhYcp1XVTQ7JHiA4n2qaV * PT-4210: give ParatextProjectSendReceiveService a LocalParatextProjects reference Adds a LocalParatextProjects constructor parameter (bound into a property by the closed-source Paratext 10 Studio overlay patch, like pdpFactory and appInfo) so the patch can call NotifyProjectsChanged after a sync rewrites Settings.xml in place — the file watcher's own docs say such in-place rewrites must be notified inline by their writer. Part of the PT-4158 deferred-items cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MhYcp1XVTQ7JHiA4n2qaV * PT-4210: public-core D1 additions — S/R timeout on breakSyncLock, RefreshAndNotifyProjectsChanged, init snapshot emit - breakSyncLock's stub registration now passes the indefinite S/R timeout itself (internal s_sendReceiveTimeout, matching the P10S overlay's name so the two unify): S/R commands are multi-minute server operations, so the request timeout must outlive them. Inert in plain Platform.Bible since the stub throws immediately. - New public virtual LocalParatextProjects.RefreshAndNotifyProjectsChanged(): RefreshScrTexts + NotifyProjectsChanged as one primitive for writers whose on-disk changes are invisible to the non-recursive project-directory watcher (in-place Settings.xml rewrites, mid-clone states). - Moved the notify-after-sync integration rationale from the SyncProjects stub body (which the overlay replaces wholesale) to the constructor comment, and pointed it at the new primitive. - SendReceiveBlockNotifierService.InitializeAsync now emits the current gate snapshot once after registration, so subscribers converge after a backend restart; tests cover the idle and already-armed cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pBpGFRLUt3fioxfz45jnU * PT-4210: review-round code fixes — event wire docs, parallel registrations, stub fault mode, renderer hardening - New OpenRpcSingleNotificationDocumentation/OpenRpcNotificationDocumentation records (C# mirror of TS SingleNotificationDocumentation) and the onSyncWriteLockChanged registration now sends them as network:registerEvent's second argument, carrying the x-experimental wire marker (review 1). - SendReceiveBlockNotifierService registers its event and command in parallel via Task.WhenAll, so a stalled main can't serialize two full request timeouts onto the startup barrier (17). - BreakSyncLock stub delivers its PlatformUnimplementedException through the returned task, matching the patched async implementation's fault mode (25). - SyncProjects' dev placeholder uses the PapiClient property instead of the raw ctor param, clearing the CS9124 double-capture warning (27). - Renderer auto-sync-blocking-service: malformed-snapshot warnings now name their source (event vs init query) and latch per source (4); projectIds validation uses .every(isString) (24); the init-consult failure docs describe the real cold-start race instead of unreachable "older cores / extension absent" scenarios, and the catch logs warn instead of debug since any rejection is anomalous on an in-repo seam (3); same reword on the seam's getAutoSyncBlocking declaration. - Doc-assertion tests per the VersificationConversionServiceTests precedent: getAutoSyncBlocking + the event registration docs in the notifier tests, and a new ParatextProjectSendReceiveServiceStubDocsTests for breakSyncLock (named to avoid the P10S overlay's test file names) (5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pBpGFRLUt3fioxfz45jnU * PT-4210: review-round docs/comments — breakSyncLock contract detail, seam re-sync guard, mock typing - breakSyncLock wire summary gains the build-agnostic availability sentence ("Only implemented in Paratext 10 Studio; throws PlatformUnimplementedException elsewhere") and the registration uses the named documentation: argument (2, 29); dropped the comment that only restated the x-experimental marker (23). - Seam d.ts: `false` means "not broken — an attempt may not have been made; a later retry can succeed" (6a); keys are upper-cased, index with upper-cased ids (7); null/blank ids are skipped and omitted (8); the header NOTE now also forbids a re-sync downgrading richer declarations that exist in both copies (10). - auto-sync-blocking-service: seed + event handler carry "typed-by-the-seam but untrusted wire data" pinning comments (16); the free-floating payload-type comment is now a one-line pointer to the seam declaration (21); noted that plain sendCommand (retry-on-timeout) is deliberate now that the command is declared — requestNoRetry was only a workaround for the missing declaration (S1). - Tests: the two well-formed snapshot mocks are pinned with `satisfies SyncWriteLockSnapshot`; deliberately-malformed ones stay untyped (15). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pBpGFRLUt3fioxfz45jnU * PT-4210: bind the patch-bound ctor params into core-side properties (D2b) Replace the CS9113 pragma (and its unread-parameter rationale comment) with read-only auto-properties PdpFactory / AppInfo / ParatextProjects in the PapiClient property's style, using exactly the names the Paratext 10 Studio patch's own bindings use so the patch can drop its binding hunk and rely on these. The params are now consumed by the initializers, so no suppression is needed; a guard comment marks the properties as patch-only reads that must not be removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pBpGFRLUt3fioxfz45jnU * PT-4210: review-round fixes — refresh/notify funnel, syncProjects timeout, awaited baseline emit, doc truth-up Four approved fixes from timothy-mccormack's 2026-07-29 review round: 1. LocalParatextProjects: RefreshAndNotifyProjectsChanged now carries the best-effort contract itself (refresh in try, notify after catch — a refresh throw must not suppress the notify) plus the class-convention _disposed early-return guard, and OnProjectDirectoriesChanged delegates to it so the inline and watcher paths share one funnel. New test pins the delegation. 2. ParatextProjectSendReceiveService: the syncProjects registration now carries s_sendReceiveTimeout like breakSyncLock, and the field's comment covers both rationales (multi-minute whole-project syncs; the lock-break's potentially N x ~100s server calls) and states that 0 is deliberately unbounded — only a lost response on a live socket waits forever, a risk every S/R command shares. 3. SendReceiveBlockNotifierService: the init baseline snapshot emit is now AWAITED (best-effort, logged on failure) so the startup barrier (Program.cs's critical Task.WhenAll) guarantees the baseline emit precedes any command-driven gate arm's event; InitializeAsync doc notes the ordering guarantee. 4. Docs, three sites (TS seam NetworkEvents entry, the C# class doc, auto-sync-blocking-service.ts): "plain Platform.Bible never emits it" was falsified by the baseline emit — now phrased as "the gate never arms in plain PB; the only plain-PB emission is a single not-blocking baseline snapshot at backend (re)start". BlockStateChanged never firing in plain PB remains stated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187X1kRaKEf634749ZDqh5A * doc: PT-4210 note breakSyncLock id trimming + case-variant collapse in @PARAM docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187X1kRaKEf634749ZDqh5A * doc: PT-4210 correct InitializeAsync baseline-emit ordering claim Task.WhenAll does not order its members, and the S/R command registrations share the startup barrier with this service, so a mid-barrier command can arm the gate and emit before the baseline — the previous doc claimed the barrier made that impossible. State what actually holds: the emit attempt completes before the barrier returns; post-barrier arms emit strictly behind it (single connection, FIFO); a mid-barrier arm that beats the baseline is reflected in it (GetBlockState() is a live read at emit time) rather than overwritten by a stale not-blocking snapshot; late subscribers seed via getAutoSyncBlocking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187X1kRaKEf634749ZDqh5A * fix: PT-4210 make debounce-timer disposal atomic with scheduling NotifyProjectsChanged and ScheduleProjectDirectoriesChanged checked _disposed before taking their debounce lock, while Dispose tore the timers down outside those locks - so a call racing Dispose could Change an already-disposed timer (ObjectDisposedException) or, worse, construct a brand-new timer after Dispose that nothing ever disposes and that later emits against a disposing PapiClient. Move each _disposed check inside its debounce lock and dispose each timer under that same lock, making check-then-schedule atomic with the teardown (deadlock-free: no timer callback holds these locks while waiting on Dispose, and Timer.Dispose() does not block on in-flight callbacks). Also give EmitProjectsChanged a _disposed back-off as a second line of defence for a callback already in flight at disposal. The race window (between check and lock) is not reachable deterministically from the existing test seams, so the invariant is documented on Dispose instead of pinned by a test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187X1kRaKEf634749ZDqh5A * doc: PT-4210 note breakSyncLock null-array behavior in @PARAM docs A null projectIds array is out of contract (the type is string[]); implementations treat it like the empty array (no-op). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187X1kRaKEf634749ZDqh5A --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
PT-4214 Stage U slice: the backend
SendReceiveWriteLockbecomes the single, per-projectblock-signal authority for ALL sync types (manual, scheduled, startup/shutdown), and the
renderer consumes it as a snapshot. Fixes two of Rolf's 2026-07-16 E2E defects at the root:
from the gate the Streamlined a number of npm scripts #164 bracket arms on every sync path — no UI path can miss it.
driver are both per-project now (previously: global latch, every Scripture editor frozen).
C# (public core, inert upstream)
SendReceiveWriteLock.EnterWriterejection is now per-project (armed ∧ projectId ∈ blocked set); the drain stays global. This deliberately reverses the "global gate"rejection semantics shipped in PT-4159: auto-sync blocking overlay (edit block for scheduled S/R) #2555/PT-4159: Rebuild S/R write gate without thread affinity #2564: an armed gate rejecting writes to projects it
isn't syncing is exactly E2E defect 3 at the data layer (once Streamlined a number of npm scripts #164 arms the gate for real),
and PT9 parity locks only the syncing project. Memory-ordering is unchanged and safe: the
volatile blocked set is published before the arming CAS, so an armed reader always observes
the matching set (reasoning documented at the check; reviewed adversarially — verdict sound).
The
EnterWrite_WhileAnotherProjectSyncs_RejectsAllProjects_GlobalGatetest was consciouslyreplaced by per-project equivalents.
BlockStateChangedevent +GetBlockState()snapshot (raised post-arm /post-real-disarm only; stale-token and no-op clears don't raise; out-of-order delivery in
off-contract crash-recovery races documented as accepted YAGNI).
SendReceiveBlockNotifierService: forwards gate transitions as network eventparatextBibleSendReceive.onSyncWriteLockChanged({ isBlocking, projectIds }snapshots) andserves
paratextBibleSendReceive.getAutoSyncBlocking— the command Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) #2571's init-seedingalready calls but nothing served until now. Deviation from PT-4214's open item: the ticket
suggested registering that command ext-side; serving it from C# is the Stage U end state (the
gate IS the authority, and it works for all sync types + before extension activation). Round 4: the event is now
formally registered with main's central event registry at init via a generic
network:registerEventrequest (that method already serves every websocket client; no newAPI — mirrors
RegisterRequestHandlerAsync'snetwork:registerMethodcall), so the boot-time“announced but is not registered” deprecation warning no longer applies to this event. Emission
itself still follows the SharedStore
SendEventAsyncprecedent; the other C#-origin events(
<id>-pdp-data:onDidUpdate,shared-store:change) remain on the legacy unregistered-announcepath — platform-wide migration debt, out of scope here.
(SR_EDIT_BLOCKED)byte-identical;SetSyncing/Clear/EnterWritesignaturesunchanged (the in-flight Streamlined a number of npm scripts #164 patch bracket compiles against this unmodified).
Renderer
auto-sync-blocking-store: ref-count latch → snapshot of blocked projectIds (200 msshow-grace kept). The Stage-T per-blocker safety leashes are deleted, not retained — per
the ratified PT-4214 §5.2 ("a second, timer-driven opinion about blocking is precisely the
drift findings 7/8/16 indict"); resilience is re-query of the authority.
auto-sync-blocking-service: subscribesonSyncWriteLockChangedonly — the ext-emittedonAutoSyncBlockingChangedsubscription is removed (single source, finding 16). Initconsult of
getAutoSyncBlockingkept (now actually served), live-event-wins guard kept,malformed payloads fail safe to unblocked.
auto-sync-edit-block-driver: flags an editor iffdefinition.projectIdis in the blockedset; handles partial set transitions ({A,B}→{A}, {A}→{B} swap) with the
unsubscribe-before-unflag ordering preserved (the PT-4159: auto-sync blocking overlay (edit block for scheduled S/R) #2555 live-E2E permanent-block bug class);
regression tests cover full clear, shrink, swap, grow, rebuilt-mid-block, no-projectId.
Runtime pairing (IMPORTANT)
The signal only fires once the studio patch arms the gate — merge in the same window as
studio #164 (gate arming + stall watchdog). Pairings:
global rejection — no regression;
loses its listener). Don't ship a build in that window.
Once merged, the ext engine's
onAutoSyncBlockingChangedemit chain(in the send-receive extension's activation + engine
setBlockingpath) is vestigial —removal tracked on PT-4214 (Stage U cleanup), not done here to keep #186's review diff stable.
Out of scope (Stage U remainder, tracked on PT-4214)
Finding 18's editor-mount refactor (driver deletion), the shared latch-store extraction,
throwOnDrainTimeoutupgrade path, seam d.ts declarations for the new event/command (deferredto avoid colliding with #2570's d.ts re-home; the renderer/service use local types).
Verification
C#
dotnet test: full suite 1528 passed / 0 failed / 6 skipped (now 1529 with the review-fixtest); SendReceive-filtered 66/0; CSharpier clean. Renderer: targeted 50/0 across
store/service/driver; full core TS suite 1018/0 including app.component.test; typecheck clean
(one pre-existing unrelated buildInfo.json error). Both halves adversarially reviewed
(finder→refuter); all surviving findings fixed in
438d1dd/506e9fc, including a truthfularmed-empty-set warning and observed (logged) notifier faults. Round 4: central-registry
registration added in
756d33e(SendReceive-filtered 67/0; full C# suite 1530/0).Rebase note (2026-07-17) — re-seated on #2571's review-fix round
Rebased onto #2571's new tip
56b9934560a(lyonsil's 19-finding review fix round: identity releasetokens in both stores, inert seeding cut, hook types, smalls). Expected supersession: #2571's
auto-sync store/service fixes conflict with this branch's snapshot rewrite and resolve to this
branch's versions — the snapshot model makes the anonymous-pairing problem the #2571 fixes
mitigated unrepresentable. One reconciliation commit added:
08d0d3d31ac— drops Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) #2571's anonymous-boolean integration test (its event seam —onAutoSyncBlockingChanged+ raise/clear pairing — no longer exists here), updates theindex.tsxstartup comment for the reintroduced async init consult (Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) #2571 review finding 13),reconciles the
AUTO_SYNC_MAX_DURATION_MSdoc (keeps Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) #2571's app-driven definition + thisbranch's leash-deletion paragraph), and commits the
papi.d.tsdrift.Post-rebase verification: renderer suite 548 passed, full typecheck clean (all workspaces), C#
suite 1530 passed / 0 failed, stack linearity verified (#2571 → #2574 → #2575).
Follow-up rebase (2026-07-17): rebased onto
37d955d1583(papi.d.ts regen fix).🤖 Generated with Claude Code
https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
Self-review — 2026-07-17 (review-paratext methodology)
No Critical/Important defects; the per-project reversal of #2564's global gate is sound and correctly re-tested. Four analysis passes (api / style / compliance / ux) per
.claude/agents/review-analyzer.md, run by a review-lead agent; author interview substituted with the PR body + PRD design records; every finding adversarially verified against code and tests.SendReceiveBlockNotifierServiceTestsdoesn't exercise the registration-failure/exception paths (DummyPapiClienthard-codes success)isProjectBlockedis exported and unit-tested but has no production consumer in this PRVerified wins: the per-project
EnterWritereversal is sound (narrowing rejection can only allow more writes, never fewer); the memory-ordering argument (volatile publish-then-flag / acquire-release pairing) is correct.Gates: C# SendReceive-filtered 67/0, renderer targeted 50/0,
typecheck:coreclean (one pre-existing unrelated build-artifact error).This change is